Skip to content

Comments

[SPARK-25817][SQL] Dataset encoder should support combination of map and product type#22812

Closed
cloud-fan wants to merge 3 commits intoapache:masterfrom
cloud-fan:map
Closed

[SPARK-25817][SQL] Dataset encoder should support combination of map and product type#22812
cloud-fan wants to merge 3 commits intoapache:masterfrom
cloud-fan:map

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Oct 24, 2018

What changes were proposed in this pull request?

After #22745 , Dataset encoder supports the combination of java bean and map type. This PR is to fix the Scala side.

The reason why it didn't work before is, CatalystToExternalMap tries to get the data type of the input map expression, while it can be unresolved and its data type is known. To fix it, we can follow UnresolvedMapObjects, to create a UnresolvedCatalystToExternalMap, and only create CatalystToExternalMap when the input map expression is resolved and the data type is known.

How was this patch tested?

enable a old test case

cloud-fan
cloud-fan commented Oct 24, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Oct 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are unrelated, but is a followup of #16986 to address the remaining code style comments.

cloud-fan
cloud-fan commented Oct 24, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to address #22745 (comment)

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 24, 2018

cc @michalsenkyr @vofque @viirya

@SparkQA
Copy link

SparkQA commented Oct 24, 2018

Test build #97959 has finished for PR 22812 at commit da31d26.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class UnresolvedCatalystToExternalMap(

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 24, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 24, 2018

Test build #97965 has finished for PR 22812 at commit da31d26.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class UnresolvedCatalystToExternalMap(

cloud-fan
cloud-fan commented Oct 25, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated but to fix minor code style issues in #22749

@SparkQA
Copy link

SparkQA commented Oct 25, 2018

Test build #98019 has finished for PR 22812 at commit bc39824.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 25, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 25, 2018

Test build #98020 has finished for PR 22812 at commit cf5a01e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 25, 2018

Test build #98026 has finished for PR 22812 at commit cf5a01e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

viirya
viirya reviewed Oct 26, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala Outdated
Copy link
Member

@viirya viirya Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated
Copy link
Member

@viirya viirya Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When u.child is resolved, is there still UnresolvedExtractValue?

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I think so. The UnresolvedExtractValue might appear in CatalystToExternalMap.keyLambdaFunction and valueLambdaFunction

Copy link
Member

@viirya viirya Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolveReferences might also process that, but it is also good to have them here.

Copy link
Contributor Author

@cloud-fan cloud-fan Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I don't quite remember why I did this for MapObjects, so I just follow it here. Maybe we can remove it in a followup PR.

@viirya
Copy link
Member

viirya commented Oct 26, 2018

LGTM

@SparkQA
Copy link

SparkQA commented Oct 26, 2018

Test build #98062 has finished for PR 22812 at commit 517bebf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 28, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 28, 2018

Test build #98147 has finished for PR 22812 at commit 517bebf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 28, 2018

thanks, merging to master!

@asfgit asfgit closed this in ff4bb83 Oct 28, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@cloud-fan
[SPARK-25817][SQL] Dataset encoder should support combination of map …
637d4f4 
…and product type

## What changes were proposed in this pull request?

After apache#22745 , Dataset encoder supports the combination of java bean and map type. This PR is to fix the Scala side.

The reason why it didn't work before is, `CatalystToExternalMap` tries to get the data type of the input map expression, while it can be unresolved and its data type is known. To fix it, we can follow `UnresolvedMapObjects`, to create a `UnresolvedCatalystToExternalMap`, and only create `CatalystToExternalMap` when the input map expression is resolved and the data type is known.

## How was this patch tested?

enable a old test case

Closes apache#22812 from cloud-fan/map.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cloud-fan @SparkQA @viirya